Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #1588 - Observable Gauge does not reflect updated values, and send the old value always #1641

Merged
merged 6 commits into from
Oct 2, 2022
Merged

Fix #1588 - Observable Gauge does not reflect updated values, and send the old value always #1641

merged 6 commits into from
Oct 2, 2022

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Sep 30, 2022 •
edited
Loading

Fixes #1588

Changes

Fixed the logic to convert monotonic increasing async counter values to the delta. Also fixed the ostream metric example to generate monotonic increasing values for the async counter.

More details -
While the observable instrument always records monotonically increasing values, it is possible for exporter/reader to request for either cumulative or delta of it. The AsyncMetricStorage::Record() method does conversion from recorded value to delta, and further passes it to TemporalMetricStorage during collection. TemporalMetricStorage further uses it to maintain the unreported values for each collector.

There was a problem in the conversion logic of recorded value -> delta in AsyncMetricStorage::Record(). This code is fixed now. And also added a test case to validate it.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team September 30, 2022 18:19
@codecov
Copy link

codecov bot commented Sep 30, 2022 •
edited
Loading

Codecov Report

Merging #1641 (a868b7c) into main (d127140) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1641      +/-   ##
==========================================
- Coverage   85.10%   85.08%   -0.02%     
==========================================
  Files         159      159              
  Lines        4991     4990       -1     
==========================================
- Hits         4247     4245       -2     
- Misses        744      745       +1
Impacted Files Coverage Δ
sdk/src/metrics/meter.cc 33.08% <ø> (ø)
sdk/src/metrics/state/temporal_metric_storage.cc 96.43% <ø> (ø)
...telemetry/sdk/metrics/state/async_metric_storage.h 86.12% <100.00%> (-0.37%) ⬇️
sdk/src/trace/batch_span_processor.cc 90.63% <0.00%> (-0.78%) ⬇️

esigo
esigo approved these changes Oct 2, 2022
View reviewed changes
Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks for the fix :)

sdk/test/metrics/async_metric_storage_test.cc Outdated Show resolved Hide resolved
@lalitb lalitb merged commit f21b3a0 into open-telemetry:main Oct 2, 2022
@tauseef-ah
Copy link

@lalitb I was testing these changes. They work fine for ObservableCounter, but Observable Gauge still send the old value. Can you pls check for Observable Gauge as well

@lalitb
Copy link
Member Author

lalitb commented Oct 3, 2022 •
edited
Loading

@tauseef-ah - Thanks for testing, I felt the change should work for all the observable instruments, but let me test it for Observable Gauge too.

@lalitb
Copy link
Member Author

lalitb commented Oct 3, 2022 •
edited
Loading

@tauseef-ah - Can you test if by setting AggergationTemporality as kDelta (default is kCumulative for ostream exporter), as it gives the desired result. I still need to check specs guidelines for the desired effect if the Temporality is set as kCumulative.
You need to change the ostream exporter creation here to change the temporality -

opentelemetry-cpp/examples/metrics_simple/metrics_ostream.cc

Line 32 in f21b3a0

std::unique_ptr<metric_sdk::MetricExporter> exporter{new exportermetrics::OStreamMetricExporter}; 

std::unique_ptr<metric_sdk::MetricExporter> exporter{new exportermetrics::OStreamMetricExporter(std::cout, AggregationTemporality::kDelta};

@reyang
Copy link
Member

reyang commented Oct 3, 2022

@tauseef-ah - Can you test if by setting AggergationTemporality as kDelta (default is kCumulative for ostream exporter), as it gives the desired result.

I guess aggregation temporality doesn't apply to Gauge since it is non-additive?

@lalitb
Copy link
Member Author

lalitb commented Oct 3, 2022 •
edited
Loading

I guess aggregation temporality doesn't apply to Gauge since it is non-additive?

Yes, I realized it afterward. There seems some issue in the cumulative pipeline while handling Gauge, I am looking into it.

@lalitb lalitb mentioned this pull request Oct 4, 2022
Merged
3 tasks
@lalitb
Copy link
Member Author

lalitb commented Oct 7, 2022

@tauseef-ah This has been fixed in #1651 if you want to test in the latest commit.

yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
@lalitb @yxue
Fix open-telemetry#1588 - Observable Gauge does not reflect updated v…
4a8c540 
…alues, and send the old value always (open-telemetry#1641)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esigo esigo esigo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Observable Gauge does not reflect updated values, and send the old value always
4 participants
@lalitb @tauseef-ah @reyang @esigo